Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ?Sized type parameters in Debug #289

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Fix ?Sized type parameters in Debug #289

merged 5 commits into from
Aug 14, 2023

Conversation

tyranron
Copy link
Collaborator

Synopsis

At the moment, derive_more::Debug fails to work with ?Sized generics:

#[derive(derive_more::Debug)]
struct UnnamedGenericStructUnsized<T: ?Sized>(T);

And generates the following error:

error[E0277]: the size for values of type `T` cannot be known at compilation time
   --> tests/debug.rs:845:14
    |
845 |     #[derive(Debug)]
    |              ^^^^^ doesn't have a size known at compile-time
846 |     struct UnnamedGenericStructUnsized<T: ?Sized>(T);
    |                                        - this type parameter needs to be `std::marker::Sized`
    |
    = note: required for the cast from `&T` to `&dyn Debug`
    = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider borrowing the value, since `&&T` can be coerced into `&dyn Debug`
    |
845 |     #[derive(&Debug)]
    |              +
help: consider removing the `?Sized` bound to make the type parameter `Sized`
    |
846 -     struct UnnamedGenericStructUnsized<T: ?Sized>(T);
846 +     struct UnnamedGenericStructUnsized<T>(T);
    |

At the same moment, std::Debug works OK:

#[derive(std::Debug)]
struct UnnamedGenericStructUnsized<T: ?Sized>(T);

If we look at its expansion:

#[automatically_derived]
impl<T: ::core::fmt::Debug + ?Sized> ::core::fmt::Debug for
    UnnamedGenericStructUnsized<T> {
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        ::core::fmt::Formatter::debug_tuple_field1_finish(f,
            "UnnamedGenericStructUnsized", &&self.0)
    }
}

We can see that std::Debug always uses fields as &&self.0, while we in our derive_more::Debug expansion use &self.0 only.

Solution

Simply use double-ref in the expansion, as the error suggests, and std::Debug does.

Checklist

  • Documentation is updated (not required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (not required)

@tyranron tyranron added this to the 1.0.0 milestone Aug 10, 2023
@tyranron tyranron self-assigned this Aug 10, 2023
@tyranron tyranron marked this pull request as draft August 10, 2023 11:14
@tyranron tyranron marked this pull request as ready for review August 10, 2023 11:31
@tyranron tyranron requested a review from JelteF August 10, 2023 11:31
@JelteF
Copy link
Owner

JelteF commented Aug 10, 2023

Does the same issue exist for other derives? Especially Display seems likely to have the same bug

@tyranron
Copy link
Collaborator Author

tyranron commented Aug 10, 2023 •
edited
Loading

@JelteF seems no. The bug goes from the &dyn Debug usage in the debug building API. Display-like and other traits derives don't use such API atm.

I can add similar tests for other traits as a separate PR. Just to be sure.

@tyranron tyranron mentioned this pull request Aug 10, 2023
Closed
@tyranron
Copy link
Collaborator Author

@JelteF ping.

@JelteF JelteF mentioned this pull request Aug 13, 2023
Closed
@tyranron tyranron merged commit 4aefdbb into master Aug 14, 2023
16 checks passed
@tyranron tyranron deleted the fix-debug-unsized branch August 14, 2023 09:54
@JelteF JelteF mentioned this pull request Jul 3, 2024
Merged
tyranron pushed a commit that referenced this pull request Jul 4, 2024
@JelteF
Don't use double ref vars in Debug derive (#380, #289, #382)
974824a 
## Synopsis

While looking into #328 I realized the current situation around Pointer
derives
and references was even weirder because we store a double-reference to
the
fields in the local variables for Debug, but not for Display. The reason
we
were doing this was because of #289.


## Solution

This stops storing a double-reference, and only adds the additional
reference
in the places where its needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelteF JelteF JelteF approved these changes

Assignees

@tyranron tyranron

Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@tyranron @JelteF